Add ERC-6909 implementations #167
Conversation
Coverage Report
Last updated: Tue, 25 Nov 2025 21:03:22 GMT for commit |
Gas ReportComparing gas usage between Summary
Details
Showing top 20 changes out of 89 total. View all 89 changes
ℹ️ About this reportThis report compares gas usage between the base branch and this PR using
To run this locally: # Generate snapshot for current branch
forge snapshot
# Compare with another branch
git checkout main
forge snapshot --diff .gas-snapshotLast updated: Tue, 25 Nov 2025 21:03:58 GMT for commit |
|
Great! Will look at this. I am glad to have an ERC-6909 implementation. |
|
Forgot to add, after I have finalised the styling and testing of the base ERC-6909, I'll add the extensions (token supply, metadata and content URI). |
|
@lumoswiz Just want you to know, I am happy this pull request was submitted, because I want Compose to provide this ERC functionality. I and/or another reviewer will get around to this pull request. |
src/token/ERC6909/LibERC6909.sol
Outdated
|
|
||
| if (_by != address(0) && !s.isOperator[_from][_by]) { | ||
| uint256 allowed = s.allowance[_from][_by][_id]; | ||
| if (allowed != type(uint256).max) s.allowance[_from][_by][_id] = allowed - _amount; |
There was a problem hiding this comment.
Please use braces here, for example:
if (allowed != type(uint256).max)) {
s.allowance[_from][_by][_id] = allowed - _amount;
}
src/token/ERC6909/ERC6909Facet.sol
Outdated
| ERC6909Storage storage s = getStorage(); | ||
| if (msg.sender != _sender && !s.isOperator[_sender][msg.sender]) { | ||
| uint256 allowed = s.allowance[_sender][msg.sender][_id]; | ||
| if (allowed != type(uint256).max) s.allowance[_sender][msg.sender][_id] = allowed - _amount; |
There was a problem hiding this comment.
Pull Request Overview
This PR adds an ERC-6909 multi-token standard implementation using the diamond storage pattern. The implementation follows a minimal style that favors arithmetic panics over custom errors, adapted from the Uniswap V4/Solmate implementation.
Key changes:
- Added
LibERC6909.solproviding internal functions (mint, burn, transfer, approve, setOperator) for ERC-6909 token operations - Added
ERC6909Facet.solexposing the public ERC-6909 interface (balanceOf, allowance, isOperator, transfer, transferFrom, approve, setOperator)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/token/ERC6909/LibERC6909.sol | Library containing ERC-6909 internal functions and diamond storage layout for multi-token operations |
| src/token/ERC6909/ERC6909Facet.sol | Facet implementing the public ERC-6909 interface using diamond storage pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/token/ERC6909/LibERC6909.sol
Outdated
|
|
||
| if (_by != address(0) && !s.isOperator[_from][_by]) { | ||
| uint256 allowed = s.allowance[_from][_by][_id]; | ||
| if (allowed != type(uint256).max) s.allowance[_from][_by][_id] = allowed - _amount; |
There was a problem hiding this comment.
This one-line if statement violates the project's style guide (STYLE.md section 5), which requires all if statements to use brackets and a newline. The statement should be reformatted as:\nsolidity\nif (allowed != type(uint256).max) {\n s.allowance[_from][_by][_id] = allowed - _amount;\n}\n
| if (allowed != type(uint256).max) s.allowance[_from][_by][_id] = allowed - _amount; | |
| if (allowed != type(uint256).max) { | |
| s.allowance[_from][_by][_id] = allowed - _amount; | |
| } |
src/token/ERC6909/ERC6909Facet.sol
Outdated
| ERC6909Storage storage s = getStorage(); | ||
| if (msg.sender != _sender && !s.isOperator[_sender][msg.sender]) { | ||
| uint256 allowed = s.allowance[_sender][msg.sender][_id]; | ||
| if (allowed != type(uint256).max) s.allowance[_sender][msg.sender][_id] = allowed - _amount; |
There was a problem hiding this comment.
This one-line if statement violates the project's style guide (STYLE.md section 5), which requires all if statements to use brackets and a newline. The statement should be reformatted as:\nsolidity\nif (allowed != type(uint256).max) {\n s.allowance[_sender][msg.sender][_id] = allowed - _amount;\n}\n
| if (allowed != type(uint256).max) s.allowance[_sender][msg.sender][_id] = allowed - _amount; | |
| if (allowed != type(uint256).max) { | |
| s.allowance[_sender][msg.sender][_id] = allowed - _amount; | |
| } |
src/token/ERC6909/LibERC6909.sol
Outdated
|
|
||
| /// @title LibERC6909 — ERC-6909 Library | ||
| /// @notice Provides internal functions and storage layout for ERC-6909 minimal multi-token logic. | ||
| /// @dev Uses ERC-8042 for storage location standardization and ERC-6093 for error conventions. |
There was a problem hiding this comment.
The comment claims to use 'ERC-6093 for error conventions', but the implementation does not define or use any custom errors (it relies on arithmetic panics instead). Either remove the ERC-6093 reference from the documentation or add appropriate custom errors as done in other token implementations (see LibERC20.sol for reference).
| /// @dev Uses ERC-8042 for storage location standardization and ERC-6093 for error conventions. | |
| /// @dev Uses ERC-8042 for storage location standardization. |
|
Thanks for the feedback, I'm on it. @mudgen do you have a preference for how to add the ERC-6909 extensions? Should I add them in this PR, or in separate PRs? I'll make the changes as above, and then add tests this week. |
|
@lumoswiz Please submit each extension as a separate pull request. That makes it easier for them to review. I look forward to the extensions. This is great. By the way, we released new documentation here: I am interested if feedback on this new documentation, if you have any feedback to give about it. |
lumoswiz
force-pushed
the
feat/erc6909
branch
from
681c035 to
24020b7
Compare
✅ Deploy Preview for compose-diamonds ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Wow, panic via arithmetic checks, instead of custom errors is so clean. Maybe we should do that for all kinds of token transfers ERC1155 and ERC20. |
Hi, it's really clever but does that break compatibility where project expect a different custom error for just wondering |
|
@maxnorm Yes it could. And I think a |
mudgen
left a comment
mudgen
left a comment
There was a problem hiding this comment.
@lumoswiz I reviewed the code and I like it. Good job.
While I like the cleanness of using the builtin Solidity arithmetic panic, I think we should use the following errors that OZ uses for this implementation:
error ERC6909InsufficientBalance(address _sender, uint256 _balance, uint256 _needed, uint256 _id);
error ERC6909InsufficientAllowance(address _spender, uint256 _allowance, uint256 _needed, uint256 _id);
error ERC6909InvalidApprover(address _approver);
error ERC6909InvalidReceiver(address _receiver);
error ERC6909InvalidSender(address _sender);
error ERC6909InvalidSpender(address _spender);Can you change the code to use these?
|
Can you also add the interface https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/interfaces/IERC6909.sol |
|
I can add the interfaces and errors. Would you like this implementation to then essentially mirror OZ? This current implementation doesn't guard against the zero address (e.g. in Without these checks, some of the errors mentioned above don't make sense to include. @mudgen @maxnorm Let me know what you think, I am happy to refactor this to more closely mirror the OZ impl. |
|
@lumoswiz But the code should be written differently than OpenZepplen. Please review the documentation for how the code should be written. Specifically:
|
|
@mudgen Added the errors and interface. The |
| if (_sender == address(0)) { | ||
| revert ERC6909InvalidSender(address(0)); | ||
| } | ||
|
|
||
| if (_receiver == address(0)) { | ||
| revert ERC6909InvalidReceiver(address(0)); | ||
| } |
There was a problem hiding this comment.
I know this is different than the OZ implementation, but please put these sender and receiver checks at the beginning of this function. Now our implementation is better than OZ.
| if (_from == address(0)) { | ||
| revert ERC6909InvalidSender(address(0)); | ||
| } | ||
|
|
||
| if (_to == address(0)) { | ||
| revert ERC6909InvalidReceiver(address(0)); | ||
| } |
There was a problem hiding this comment.
Please put these functions at the beginning of the function.
…et and LibERC6909
Co-authored-by: Copilot <[email protected]>
|
Good job! |
|
@lumoswiz We have an ERC6909 test failing. Can you check it out? |
3 participants
Summary
Add
ERC6909FacetandLibERC6909implementations adhering to ERC-6909.A minimal styling favouring panic via arithmetic checks in lieu of custom errors was used here. Please see additional notes below for alternative styles.
Testing to be done over the next few days.
Changes Made
Added:
ERC6909Facet.solLibERC6909.solChecklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtTests are included - All new functionality has comprehensive tests
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the CONTRIBUTING.md guidelines.
Additional Notes
There are multiple ERC-6909 implementations available, with slightly different styles. See links below:
I have opted for the minimal Uniswap V4/Solmate implementation, however, I am happy to adapt to whatever styling you prefer. Please let me know.